Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add support for resources when packaging using the SwiftPM plugin #333

Merged
merged 6 commits into from
Aug 5, 2024
Merged

Add support for resources when packaging using the SwiftPM plugin #333

merged 6 commits into from
Aug 5, 2024

Conversation

camdenfullmer
Copy link
Contributor

This change updates the SwiftPM plugin to include the executable's resources in the archive.

Motivation:

When deploying a Lambda function that generates HTML content I needed a way to package template HTML files along with the executable. Defining the resources in the Package.swift and then packaging them in the archive in a way so that Swift can access them using Bundle.main.resourceURL felt like the best way to accomplish this.

Example:

.executableTarget(
            name: "Website",
            dependencies: [
                .product(name: "AWSLambdaRuntime", package: "swift-aws-lambda-runtime"),
            ],
            resources: [
                .process("template.stencil"),
            ]),

Modifications:

Plugin code has been modified to generate a Contents/Resources directory inside the final archive which includes the resources defined in the executable's target from Package.swift.

Result:

The zip archive includes the defined resources in Package.swift.

@sebsto sebsto self-assigned this Jun 22, 2024
@sebsto sebsto added the enhancement New feature or request label Jun 22, 2024
@sebsto
Copy link
Contributor

sebsto commented Jun 22, 2024

Looks like this address issue #312

@sebsto
Copy link
Contributor

sebsto commented Jun 22, 2024

@camdenfullmer Thank you for this PR. It looks like a legitimate idea and simple implementation

I would propose to add a minimal example Lambda function to show how to use this capability and a section in the README file to document this.

Question: why Contents/Resources and not just Resources like the mac bundles ? (I know, naming things is hard)

I left comments on the code

sebsto
sebsto reviewed Jun 22, 2024
View reviewed changes
Copy link
Contributor

@sebsto sebsto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few questions and suggestions. Thank you for having submitted this

Plugins/AWSLambdaPackager/Plugin.swift Outdated
@@ -202,8 +202,27 @@ struct AWSLambdaPackager: CommandPlugin {
try FileManager.default.copyItem(atPath: artifactPath.string, toPath: relocatedArtifactPath.string)
try FileManager.default.createSymbolicLink(atPath: symbolicLinkPath.string, withDestinationPath: relocatedArtifactPath.lastComponent)

// add resources
let contentsDirectory = workingDirectory.appending("Contents")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we really need a Contents directory ? Maybe Resources is enough

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After some more testing the correct bundle to use is Bundle.module. So instead of recreating the Contents directory I am just copying the .resources directory that is in the artifacts directory to the working directory to be zipped. This has the benefit of working on Lambda as well as when running locally on macOS.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://developer.apple.com/documentation/xcode/bundling-resources-with-a-swift-package#Access-a-resource-in-code

Plugins/AWSLambdaPackager/Plugin.swift Outdated
let artifactDirectory = artifactPath.removingLastComponent()
let resourcesDirectoryName = try FileManager.default.contentsOfDirectory(atPath: artifactDirectory.string)
.first(where: { $0.hasSuffix(".resources") && $0.contains(product.name) })
if let resourcesDirectoryName {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would use guard or if on line 210. This line is not necessary

@sebsto
Copy link
Contributor

sebsto commented Jun 22, 2024

@swift-server-bot test this please

@sebsto sebsto mentioned this pull request Jun 22, 2024
Closed
@sebsto
Copy link
Contributor

sebsto commented Aug 1, 2024

@swift-server-bot test this please

@sebsto
Copy link
Contributor

sebsto commented Aug 1, 2024

@camdenfullmer Thank you for having submitted this PR. We're considering merging it in the main branch before we start work on v2.
Can you address the two suggestions left above ?

@camdenfullmer
Copy link
Contributor Author

@camdenfullmer Thank you for having submitted this PR. We're considering merging it in the main branch before we start work on v2. Can you address the two suggestions left above ?

Yep will get right on it. Will try to have changes and feedback by the end of the week!

camdenfullmer
camdenfullmer commented Aug 1, 2024
View reviewed changes
Plugins/AWSLambdaPackager/Plugin.swift
@@ -67,6 +67,7 @@ struct AWSLambdaPackager: CommandPlugin {

// create the archive
let archives = try self.package(
packageName: context.package.displayName,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wasn't sure if there was a better way to get the package name when creating the resources directory name.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the only way to retrieve a package name.
See https://github.com/swiftlang/swift-package-manager/blob/b24cc8b547f952ee4a954db939f115a26946fe2b/Sources/PackagePlugin/PackageModel.swift#L16

The other option is to use the directory name but it's not necessarily the same.

@sebsto
Copy link
Contributor

sebsto commented Aug 1, 2024

@swift-server-bot test this please

@sebsto
Copy link
Contributor

sebsto commented Aug 1, 2024

@camdenfullmer Thank you for the update. I tested the code on different variations of your sample project. It works like a charm,
Can you make sure the scripts/soundness.sh script runs without error ? It's just the license headers missing from the example project.

19:06:17    * swift-or-c... missing headers in file './Examples/ResourcePackaging/Lambda.swift'!

@camdenfullmer
Copy link
Contributor Author

@camdenfullmer Thank you for the update. I tested the code on different variations of your sample project. It works like a charm, Can you make sure the scripts/soundness.sh script runs without error ? It's just the license headers missing from the example project.

19:06:17    * swift-or-c... missing headers in file './Examples/ResourcePackaging/Lambda.swift'!

I changed the year to something that will pass, but maybe the script should be updated to allow for 2024?

@sebsto
Copy link
Contributor

sebsto commented Aug 5, 2024

@swift-server-bot test this please

sebsto
sebsto reviewed Aug 5, 2024
View reviewed changes
Copy link
Contributor

@sebsto sebsto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@sebsto sebsto merged commit 35e0919 into swift-server:main Aug 5, 2024
7 of 8 checks passed
@jbelkins jbelkins mentioned this pull request Oct 4, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sebsto sebsto sebsto approved these changes

Assignees

@sebsto sebsto

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@camdenfullmer @sebsto